refactor!: Rename --print flag to --output-scan
#103
Merged
+4
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flavio
approved these changes
Sep 4, 2023
cmd/root.go
Outdated
| @@ -124,7 +124,7 @@ func init() { | |||
| rootCmd.Flags().StringP("kubewarden-namespace", "k", defaultKubewardenNamespace, "namespace where the Kubewarden components (e.g. PolicyServer) are installed (required)") | |||
| rootCmd.Flags().StringP("policy-server-url", "u", "", "URI to the PolicyServers the Audit Scanner will query. Example: https://localhost:3000. Useful for out-of-cluster debugging") | |||
| rootCmd.Flags().VarP(&level, "loglevel", "l", fmt.Sprintf("level of the logs. Supported values are: %v", logconfig.SupportedValues)) | |||
| rootCmd.Flags().BoolVarP(&printJSON, "print", "p", false, "print result of scan in JSON to stdout") | |||
| rootCmd.Flags().BoolVarP(&printJSON, "output-scan", "o", false, "output result of scan to stdout in JSON upon completion") | |||
There was a problem hiding this comment.
Nitpicking, can we use log-fmt like policy server does?
fabriziosestito
approved these changes
Sep 5, 2023
This was referenced Sep 5, 2023
nnelas
added a commit
to nnelas/helm-charts
that referenced
this pull request
Sep 6, 2023
- Removes customisation of the CA cert since this is generated by `kubewarden-controller` and there's no way to provide a custom one. - Aligns `--print` flag with kubewarden/audit-scanner#103
viccuad
force-pushed
the
main
branch
2 times, most recently
from
92f2318
to
d453e52
Compare
|
Sorry for lagging on this PR. Refactored, the behaviour now is as follows: Passing incorrect Passing correct With Cobra, I couldn't find a sane way to allow for a |
viccuad
changed the title
--print flag to --output-scan--print flag to --log-fmt, support json
jvanz
reviewed
Sep 6, 2023
cmd/root.go
Outdated
| return err | ||
| } | ||
| switch logFormat { | ||
| // TODO use slices package in go 1.21 |
There was a problem hiding this comment.
I think we can start to use it already. In the CA changes PR I've updated the go to 1.21 as well.
jvanz
approved these changes
Sep 6, 2023
flavio
reviewed
Sep 6, 2023
cmd/root.go
Outdated
| var logFormat string | ||
|
|
||
| // list of supported format of logs to stdout & stderr | ||
| var logFormatSupportedFormats = [1]string{"json"} |
There was a problem hiding this comment.
Shouldn't we have also a text format, which would be the current one?
There was a problem hiding this comment.
but, the current one outputs a json per line (see the output on previous comment). Maybe I'm misunderstanding here.
There was a problem hiding this comment.
Prior to this PR, the output was not JSON-encoded, am I right? It was something like
DATETIME - WARN - foo bar
This would be the default format to use, the text one.
To go back to policy-server, when no --log-fmt is not provided the log is in "human"/text mode. A different format can be picked by specifying --log-fmt <FMT>. This is how audit-scanner will work once this PR is merged. I just propose to add text as one of the possible values of log-fmt
There was a problem hiding this comment.
no, the output was always JSON, for the logs from zerolog, or the scan output.
zerolog seems to only support JSON or a binary format. This would apply to normal logs.
For the scan result, I'm afraid that it can be quite verbose for logs, so IMHO it should be disabled by default. That's the current behaviour and the one from this PR.
Before this PR one needed to do --print to print the result of the scan in JSON.
With this PR, one needs to do --log-fmt=json to print the result of the scan in JSON. We could add other supported formats to the scan output, but to have the same output format for normal logs, we would need to refactor the use of zerolog of find an alternative.
There was a problem hiding this comment.
Sorry, I really misunderstood the whole thing. As discussed, let's go back to the original implementation (so --print), but with a better name of the flag (like --print-report). It would be a boolean flag, disabled by default. Once enabled it would print to the stdout the report using as a JSON object
There was a problem hiding this comment.
Uook, no problem! Renamed from --print to --output-scan which was the first approach and matches kubewarden/helm-charts#282.
cmd/root.go
Outdated
| return err | ||
| } | ||
| switch logFormat { | ||
| // TODO use slices package in go 1.21 |
viccuad
force-pushed
the
main
branch
2 times, most recently
from
60c8f95
to
f732d6c
Compare
viccuad
changed the title
--print flag to --log-fmt, support json--print flag to --output-scan
We consider this flag consumable by end users from now on. Signed-off-by: Víctor Cuadrado Juan <[email protected]>
flavio
pushed a commit
to kubewarden/helm-charts
that referenced
this pull request
Sep 7, 2023
- Removes customisation of the CA cert since this is generated by `kubewarden-controller` and there's no way to provide a custom one. - Aligns `--print` flag with kubewarden/audit-scanner#103
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Relates to kubewarden/helm-charts#282.
Relates to #102.
Test
No need.
Additional Information
This signifies that using
--output-scancan be consumed by end users.I have checked the source code to ensure that the audit-scanner binary doesn't print to stdout or stderr in a way that isn't JSON formatted (unless it suddenly catches a panic and one gets an exception).
Also, the current config of
golangci-lintforbidsfmt.Printlnand the like.This should provide reasonable certainty that stdout can be consumed by end users.
Tradeoff
Potential improvement